-
-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add req res data warn #1333
Add req res data warn #1333
Conversation
Thinking maybe if we could use https://nodejs.org/dist/latest-v16.x/docs/api/util.html#util_util_deprecate_fn_msg_code It is a one time warning function... |
I feel like that's more for deprecated functions specifically, due to the node flags for it. Plus, I can't really see how exactly the deprecation function would be used here, unless No-op functions are used, but that kinda defeats the purpose of the deprecation wrapper. |
I thought so too... But it's mere elegant in a way also. + We are already using it |
I tried using the deprecate wrapper just now, and it's kinda hard to test for, should I remove the warn tests, as they aren't that critical? I don't think there is a good way of checking for |
we don't have a test for the other warning message about https://www.geeksforgeeks.org/node-js-process-warning-event/ |
Changed the tests and the warnings to use the deprecate API. |
clone: {enumerable: true}, | ||
data: {get: deprecate(() => {}, | ||
'.data is not a valid Response property, use .json(), .text(), .arrayBuffer(), or .body instead', | ||
'https://github.com/node-fetch/node-fetch/issues/1000 (response)')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall i like this PR but wonder if not this getter fn should not be in the body mixin instead.
It's possible to use the Request too new Request(url, {body}).arrayBuffer()
all doe i don't thing anyone will make that misstake... seems almost unnecessary, nobody would do: new Request().data
or assigning a value onto it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat 👍
made some small changes to this in #1421 |
What is the purpose of this pull request?
What changes did you make? (provide an overview)
Added a one-time warning for when .data is used in any way, as a way to help porting from other libraries for requests (such as
axios
).Which issue (if any) does this pull request address?
Closes #1000